feat: add some pagination options #980
Conversation
WalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Storage
Client->>API: GET /v2/{ledger}/accounts?sort=first_usage:asc
API->>API: Parse 'sort' parameter (field + order)
API->>Storage: Query accounts, paginate/sort by first_usage ASC
Storage-->>API: Accounts list (sorted, paginated)
API-->>Client: Response with accounts (includes firstUsage)
sequenceDiagram
participant Client
participant API
participant Storage
Client->>API: GET /v2/{ledger}/logs?sort=date:desc
API->>API: Parse 'sort' parameter (field + order)
API->>Storage: Query logs, paginate/sort by date DESC
Storage-->>API: Logs list (sorted, paginated)
API-->>Client: Response with logs
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
gfyrag
force-pushed
the
feat/add-pagination-options
branch
from
0545e6a to
ffcd47d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #980 +/- ##
==========================================
- Coverage 82.98% 82.91% -0.07%
==========================================
Files 145 145
Lines 8199 8219 +20
==========================================
+ Hits 6804 6815 +11
- Misses 1071 1079 +8
- Partials 324 325 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Bug
The UpdateAccountsMetadata method now accepts a time.Time parameter (at) to set FirstUsage. This introduces two issues with the SQL update logic for first_usage:
- When
time.Time{}(zero time, January 1, year 1) is passed forat(e.g., in test calls or metadata-only updates), thecase when excluded.first_usage < accounts.first_usage then excluded.first_usage else accounts.first_usage endcondition incorrectly setsfirst_usageto zero time, as zero time is always earlier than any valid timestamp. This can overwrite existing correctfirst_usagevalues. - The same SQL logic fails to correctly set
first_usagewhenaccounts.first_usageisNULLandexcluded.first_usagehas a real timestamp. The comparisonreal_date < NULLevaluates toNULL(unknown), causing theCASEstatement to retain theNULLvalue.
These issues lead tofirst_usagebeing corrupted or not set properly, preventing accurate tracking of account first usage.
internal/storage/ledger/accounts.go#L24-L62
ledger/internal/storage/ledger/accounts.go
Lines 24 to 62 in ffcd47d
Was this report helpful? Give feedback by reacting with 👍 or 👎
gfyrag
force-pushed
the
feat/add-pagination-options
branch
from
ffcd47d to
6fd34bf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
docs/api/README.md (3)
139-153: Document the exact list of sortable fields and default orderThe new
sortparameter is introduced but the doc omits:
• the exhaustive list of fields that are accepted for this endpoint,
• what happens when the caller omits<order>(isascassumed?),
• case-sensitivity expectations (asc|ASC?),
• behaviour when an unknown field is supplied.Without this information the contract is underspecified and client implementations will diverge.
1164-1180: Deprecation notice needs stronger signalling and cross-param rules
- Mark the
orderparameter with a clear “DEPRECATED” badge (orx-deprecatedin the OpenAPI spec) so generators surface it.- State whether
order,reverse, andsortare mutually exclusive and which one wins if both are provided.
1879-1893: Volumes endpoint: clarify sortable fieldsVolumes expose composite objects (
account,asset,input, …). List which of these are sortable and whether secondary ordering is guaranteed for ties.
♻️ Duplicate comments (1)
docs/api/README.md (1)
744-758: Repeat the same clarification for the accounts endpointSame gaps as noted above: accepted fields, default order, error semantics. Make sure the wording is consistent across all endpoints to avoid copy-paste drift.
🧹 Nitpick comments (6)
pkg/client/docs/models/operations/v2listtransactionsrequest.md (1)
13-13: Fix markdown formatting issues.The static analysis tool correctly identifies spacing issues within the emphasis markers (strikethrough formatting).
Apply this diff to fix the formatting:
-| ~~`Order`~~ | [*operations.Order](../../models/operations/order.md) | :heavy_minus_sign: | : warning: ** DEPRECATED **: This will be removed in a future release, please migrate away from it as soon as possible.<br/><br/>Deprecated: Use sort param | | +| ~~`Order`~~ | [*operations.Order](../../models/operations/order.md) | :heavy_minus_sign: | :warning: **DEPRECATED**: This will be removed in a future release, please migrate away from it as soon as possible.<br/><br/>Deprecated: Use sort param | |test/e2e/api_accounts_list_test.go (2)
166-176: Enhance test to verify sorting behavior.The test verifies that the API call with
sort=first_usage:ascsucceeds, but doesn't validate that the results are actually sorted correctly. Consider adding assertions to verify the accounts are returned in the expected order.It("should be listed on api while paginating on first usage", func(specContext SpecContext) { - _, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.ListAccounts( + response, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.ListAccounts( ctx, operations.V2ListAccountsRequest{ Ledger: "default", Expand: pointer.For("volumes"), Sort: pointer.For("first_usage:asc"), }, ) Expect(err).ToNot(HaveOccurred()) + + // Verify accounts are sorted by first_usage in ascending order + accounts := response.V2AccountsCursorResponse.Cursor.Data + for i := 1; i < len(accounts); i++ { + if accounts[i-1].FirstUsage != nil && accounts[i].FirstUsage != nil { + Expect(accounts[i-1].FirstUsage.Before(*accounts[i].FirstUsage) || + accounts[i-1].FirstUsage.Equal(*accounts[i].FirstUsage)).To(BeTrue()) + } + } })
323-347: Refactor to eliminate code duplication.The
listAccountshelper function normalizesFirstUsagefields, but similar normalization code exists in theBeforeEachblock, creating duplication.listAccounts := func(specContext SpecContext, request operations.V2ListAccountsRequest) *operations.V2ListAccountsResponse { GinkgoHelper() response, err = Wait(specContext, DeferClient(testServer)).Ledger.V2.ListAccounts(ctx, request) Expect(err).ToNot(HaveOccurred()) // We don't have firstUsage value, so we set it to nil to be able to compare accounts list for ind, account := range response.V2AccountsCursorResponse.Cursor.Data { account.FirstUsage = nil response.V2AccountsCursorResponse.Cursor.Data[ind] = account } return response } BeforeEach(func(specContext SpecContext) { response = listAccounts(specContext, operations.V2ListAccountsRequest{ Ledger: "default", PageSize: pointer.For(pageSize), }) - // We don't have firstUsage value, so we set it to nil to be able to compare accounts list - for ind, account := range response.V2AccountsCursorResponse.Cursor.Data { - account.FirstUsage = nil - response.V2AccountsCursorResponse.Cursor.Data[ind] = account - } Expect(response.V2AccountsCursorResponse.Cursor.HasMore).To(BeTrue()) Expect(response.V2AccountsCursorResponse.Cursor.Previous).To(BeNil()) Expect(response.V2AccountsCursorResponse.Cursor.Next).NotTo(BeNil()) })docs/api/README.md (3)
776-777: Align example timestamps
firstUsagehere uses2023-01-01T00:00:00Zwhereas the rest of the examples still use the2019-08-24T14:15:22Zconvention. Pick one convention so users don’t wonder if the value carries a special meaning.
861-862: Add a description tofirstUsageThe schema table later shows
"none"in the description column. A short explanation such as “ISO-8601 timestamp of the first recorded transaction involving this account” will improve discoverability.
2550-2556: Schema table missingfirstUsagedescriptionSame as earlier comment: add a concise description. Also double-check that the property is flagged as
readOnlyin the OpenAPI spec if clients are not supposed to supply it.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumopenapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.logpkg/client/speakeasyusagegen/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.logtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (47)
docs/api/README.md(16 hunks)internal/README.md(1 hunks)internal/account.go(1 hunks)internal/api/common/errors.go(2 hunks)internal/api/v2/common.go(2 hunks)internal/api/v2/controllers_accounts_list.go(1 hunks)internal/api/v2/controllers_ledgers_list.go(1 hunks)internal/api/v2/controllers_logs_list.go(1 hunks)internal/api/v2/controllers_transactions_list.go(1 hunks)internal/api/v2/controllers_volumes.go(1 hunks)internal/api/v2/views_test.go(2 hunks)internal/controller/ledger/controller_default.go(1 hunks)internal/controller/ledger/store.go(1 hunks)internal/controller/ledger/store_generated_test.go(1 hunks)internal/storage/common/errors.go(1 hunks)internal/storage/common/resource.go(1 hunks)internal/storage/common/schema.go(1 hunks)internal/storage/ledger/accounts.go(4 hunks)internal/storage/ledger/accounts_test.go(3 hunks)internal/storage/ledger/balances_test.go(2 hunks)internal/storage/ledger/resource_accounts.go(1 hunks)internal/storage/ledger/resource_aggregated_balances.go(1 hunks)internal/storage/ledger/resource_logs.go(1 hunks)internal/storage/ledger/resource_transactions.go(1 hunks)internal/storage/ledger/resource_volumes.go(1 hunks)internal/storage/ledger/volumes_test.go(2 hunks)internal/storage/system/resource_ledgers.go(1 hunks)pkg/client/docs/models/components/v2account.md(1 hunks)pkg/client/docs/models/operations/order.md(1 hunks)pkg/client/docs/models/operations/v2getvolumeswithbalancesrequest.md(1 hunks)pkg/client/docs/models/operations/v2listaccountsrequest.md(1 hunks)pkg/client/docs/models/operations/v2listledgersrequest.md(1 hunks)pkg/client/docs/models/operations/v2listlogsrequest.md(1 hunks)pkg/client/docs/models/operations/v2listtransactionsrequest.md(1 hunks)pkg/client/docs/sdks/v2/README.md(5 hunks)pkg/client/models/components/v2account.go(2 hunks)pkg/client/models/operations/v2getvolumeswithbalances.go(2 hunks)pkg/client/models/operations/v2listaccounts.go(2 hunks)pkg/client/models/operations/v2listledgers.go(2 hunks)pkg/client/models/operations/v2listlogs.go(2 hunks)pkg/client/models/operations/v2listtransactions.go(3 hunks)test/e2e/api_accounts_list_test.go(9 hunks)test/e2e/api_accounts_metadata_test.go(4 hunks)test/e2e/api_logs_list_test.go(1 hunks)test/e2e/api_transactions_create_test.go(3 hunks)test/e2e/api_transactions_list_test.go(2 hunks)test/e2e/api_volumes_test.go(7 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
pkg/client/docs/models/operations/v2listtransactionsrequest.md
13-13: Spaces inside emphasis markers
null
(MD037, no-space-in-emphasis)
13-13: Spaces inside emphasis markers
null
(MD037, no-space-in-emphasis)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (59)
internal/api/v2/views_test.go (1)
384-384: Test updates correctly reflect the newfirstUsagefield exposure.The addition of
"firstUsage"to the expected JSON output properly aligns with the change ininternal/account.gowhere theFirstUsagefield's JSON tag was updated from"-"to"firstUsage". This ensures test coverage accurately validates the API response structure.Also applies to: 411-411
pkg/client/docs/models/operations/order.md (1)
3-4: Clear deprecation notice provides proper migration guidance.The deprecation notice is well-formatted and clearly directs users to the new
sortparameter, supporting the transition from the legacyorderparameter. This follows good API versioning practices.internal/account.go (1)
19-19: Field exposure change implemented correctly.Changing the JSON tag from
"-"to"firstUsage"properly exposes the FirstUsage timestamp in API responses. The database mapping remains unchanged, ensuring data consistency. This is a breaking API change that should be documented in release notes.pkg/client/docs/models/components/v2account.md (1)
10-10: Documentation accurately reflects the new field addition.The
FirstUsagefield documentation is properly formatted with correct type information, optional designation, and a valid RFC3339 timestamp example. This aligns with the implementation changes in the Account model.internal/controller/ledger/controller_default.go (1)
277-277: Verify method signature consistency across the codebase.The addition of
log.Dateas a third argument toUpdateAccountsMetadataappears to align with an updated method signature. Ensure this change is consistently applied everywhere this method is called.#!/bin/bash # Search for all calls to UpdateAccountsMetadata to verify signature consistency rg -A 3 -B 1 "UpdateAccountsMetadata"pkg/client/docs/models/operations/v2listlogsrequest.md (1)
12-12: Documentation correctly added for new Sort parameter.The new
Sortfield is properly documented with clear format specification and a valid example. The field type, requirement status, and description are consistent with the existing documentation style.pkg/client/docs/models/operations/v2listledgersrequest.md (1)
10-10: Consistent documentation for Sort parameter.The
Sortfield documentation matches the pattern established in other request models, ensuring API consistency across endpoints.internal/api/v2/controllers_transactions_list.go (1)
33-33: Good refactoring to consolidate error handling.Replacing specific error type checks with
common.HandleCommonPaginationErrorsreduces code duplication and ensures consistent error handling across controllers. This aligns well with the DRY principle.internal/storage/ledger/volumes_test.go (2)
49-49: Method signature updated correctly.The addition of
time.Time{}parameter aligns with the updatedUpdateAccountsMetadatamethod signature. Using zero time appears appropriate for test scenarios where the timestamp context is not critical.
545-545: Consistent signature update.This change matches the previous update at line 49, maintaining consistency in the test file.
pkg/client/docs/models/operations/v2listaccountsrequest.md (1)
13-13: Complete and consistent Sort parameter documentation.The
Sortfield documentation maintains consistency across all list request models, providing a unified API experience for pagination and sorting functionality.pkg/client/docs/sdks/v2/README.md (1)
65-65: Documentation updates look good and consistent.The SDK documentation has been properly updated to demonstrate the new
Sortparameter across all relevant operations. The consistent use of"id:desc"as the example value provides clear guidance for developers on the expected format.Also applies to: 568-568, 940-940, 1419-1419, 1484-1484
internal/storage/ledger/resource_logs.go (1)
17-18: Schema changes correctly enable pagination support.Marking the "date" and "id" fields as paginated is the appropriate way to enable sorting and pagination functionality for these fields. This aligns well with the broader API enhancements for pagination support.
internal/storage/ledger/resource_volumes.go (1)
20-26: Schema changes correctly enable pagination support for the address field.The addition of
.Paginated()to the "address" field properly enables pagination and sorting functionality. The code formatting improvements also enhance readability.test/e2e/api_volumes_test.go (1)
86-86: LGTM: Improved test description consistency.The changes to test descriptions improve consistency by using lowercase style throughout the test suite. The test logic and coverage remain unchanged.
Also applies to: 115-115, 143-143, 170-170, 198-198, 216-216, 231-231
internal/storage/common/resource.go (1)
287-289: Excellent validation enhancement for pagination security.This validation properly prevents pagination on fields that aren't marked as paginated, improving both security and correctness. The error handling follows established patterns and provides clear feedback to consumers.
pkg/client/docs/models/operations/v2getvolumeswithbalancesrequest.md (1)
15-15: Clear and well-documented Sort parameter.The documentation clearly specifies the Sort parameter format and provides a helpful example. The
<field>:<order>format is intuitive and follows common API conventions.internal/storage/common/schema.go (1)
28-28: Clean implementation of pagination field marking.The addition of the
IsPaginatedfield andPaginated()method follows established patterns in the codebase. The fluent interface design is consistent with existing methods likeWithAliases().Also applies to: 36-39
internal/api/v2/controllers_logs_list.go (1)
23-23: Good refactoring to centralize pagination error handling.Delegating pagination error handling to
common.HandleCommonPaginationErrorsimproves consistency across controllers and reduces code duplication. This maintains the same error handling behavior while being more maintainable.internal/storage/ledger/accounts_test.go (1)
52-52: LGTM: Method signature updates are consistent.The calls to
UpdateAccountsMetadatahave been consistently updated to include the newtime.Time{}parameter, aligning with the method signature change that supports FirstUsage timestamp handling.Also applies to: 302-302, 333-333
internal/storage/ledger/balances_test.go (1)
199-199: LGTM: Consistent method signature updates.The
UpdateAccountsMetadatacalls have been properly updated to include the newtime.Time{}parameter, maintaining consistency with the signature changes applied across the test suite.Also applies to: 213-213
internal/api/v2/controllers_ledgers_list.go (1)
23-23: LGTM: Improved error handling consolidation.The delegation to
common.HandleCommonPaginationErrorsunifies error handling across API endpoints and aligns with the pagination enhancement goals of this PR.internal/storage/ledger/resource_aggregated_balances.go (1)
18-18: LGTM: Address field marked for pagination support.Adding
.Paginated()to the address field enables sorting and pagination capabilities for this commonly queried field, supporting the PR's pagination enhancement objectives.internal/README.md (1)
235-235: LGTM: FirstUsage field exposed for JSON serialization.The change from
json:"-"tojson:"firstUsage"properly exposes the FirstUsage timestamp field for API responses, enabling sorting capabilities on this field as part of the pagination enhancements.internal/api/v2/controllers_volumes.go (1)
78-78: LGTM: Error handling consolidation improves consistency.The replacement of specific error checks with
common.HandleCommonPaginationErrorsaligns with the broader refactoring pattern across controllers and improves code maintainability.internal/storage/system/resource_ledgers.go (1)
26-26: LGTM: Schema enhancement supports new pagination features.Marking the "id" field as paginated enables sorting and pagination on this field, which is consistent with the broader enhancement to support the new
sortquery parameter across endpoints.pkg/client/models/operations/v2listlogs.go (2)
22-27: LGTM: Well-documented sort parameter addition.The new
Sortfield is properly documented with clear format specification (<field>:<order>), making it easy for API consumers to understand the expected input format.
70-75: LGTM: Nil-safe getter method.The
GetSort()method correctly implements nil-safety, returningnilwhen the receiver isnil, which is consistent with the pattern used in other getter methods.internal/api/v2/controllers_accounts_list.go (1)
23-23: LGTM: Consistent error handling pattern.The use of
common.HandleCommonPaginationErrorsmaintains consistency with other controllers and improves error handling maintainability across the codebase.internal/controller/ledger/store.go (1)
45-45: ```shell
#!/bin/bash
echo "=== Verifying test call signatures in volumes_test.go ==="
rg -n "store.UpdateAccountsMetadata" -C 2 internal/storage/ledger/volumes_test.goecho "=== Verifying test call signatures in balances_test.go ==="
rg -n "store.UpdateAccountsMetadata" -C 2 internal/storage/ledger/balances_test.go</details> <details> <summary>internal/storage/ledger/resource_accounts.go (1)</summary> `19-20`: **LGTM: Schema fields correctly marked as paginated.** The addition of `.Paginated()` to both "address" and "first_usage" fields properly enables sorting and pagination support for these fields, which aligns with the PR's pagination enhancement objectives. </details> <details> <summary>pkg/client/docs/models/operations/v2listtransactionsrequest.md (1)</summary> `15-15`: **LGTM: Sort field properly documented.** The new Sort field documentation clearly explains the format (`<field>:<order>`) and provides a helpful example (`id:desc`), making it easy for API consumers to understand how to use the new sorting functionality. </details> <details> <summary>pkg/client/models/operations/v2getvolumeswithbalances.go (2)</summary> `28-32`: **LGTM: Sort field properly implemented.** The Sort field is correctly added with appropriate documentation, query parameter tags, and follows the established pattern for optional fields in the request struct. --- `96-101`: **LGTM: Getter method follows established pattern.** The GetSort() method correctly implements nil safety checks consistent with other getter methods in the struct, ensuring safe access to the Sort field. </details> <details> <summary>test/e2e/api_transactions_create_test.go (1)</summary> `127-127`: **LGTM: Test assertions properly validate FirstUsage field.** The test updates correctly verify that the FirstUsage field is present and properly populated in account responses, ensuring the new timestamp functionality works as expected across different transaction scenarios. Also applies to: 140-140, 282-282 </details> <details> <summary>internal/api/common/errors.go (2)</summary> `5-5`: **LGTM: Appropriate import added for error handling.** The storagecommon import is correctly added to support the new pagination error types used in the HandleCommonPaginationErrors function. --- `56-65`: **LGTM: Well-designed error handling consolidation.** The HandleCommonPaginationErrors function effectively centralizes pagination-related error handling by covering the key error types: - `ErrInvalidQuery` for malformed pagination queries - `ErrMissingFeature` for feature availability issues - `ErrNotPaginatedField` for attempts to paginate on unsupported fields This consolidation improves code maintainability and ensures consistent error responses across different API controllers. </details> <details> <summary>pkg/client/models/operations/v2listledgers.go (1)</summary> `18-45`: **LGTM! Well-implemented sorting parameter.** The `Sort` field addition follows the established patterns in the struct with proper query parameter annotations, clear documentation, and consistent getter method implementation. The format specification `<field>:<order>` is clear and the nil-safety in the getter matches other methods. </details> <details> <summary>internal/storage/ledger/resource_transactions.go (1)</summary> `22-24`: **LGTM! Appropriate fields marked for pagination.** Marking `timestamp` and `id` fields as paginated is logical for transaction sorting. These are the most commonly used fields for ordering transaction lists and align well with the new sorting functionality. </details> <details> <summary>pkg/client/models/operations/v2listaccounts.go (1)</summary> `22-83`: **LGTM! Consistent sorting implementation across endpoints.** The `Sort` field implementation for accounts follows the same pattern as other list operations, maintaining consistency across the API. The documentation and getter method are properly implemented. </details> <details> <summary>test/e2e/api_accounts_metadata_test.go (1)</summary> `77-78`: **LGTM! Appropriate test adaptation for schema changes.** The change from full struct equality checks to individual field assertions is a good approach to handle the addition of the `FirstUsage` field to the account schema. The tests continue to validate the core metadata functionality while being more resilient to schema additions. Also applies to: 107-108, 128-129, 155-156 </details> <details> <summary>test/e2e/api_transactions_list_test.go (2)</summary> `130-158`: **LGTM! Comprehensive test coverage for sorting functionality.** The new test case thoroughly validates the `Sort` parameter with `timestamp:desc` ordering. The test setup properly sorts expected data locally and compares it with the API response, ensuring the sorting logic works correctly end-to-end. --- `240-240`: **LGTM! Appropriate handling of deprecated field.** The `//nolint:staticcheck` directive is correctly used to suppress warnings for the deprecated `Order` field while maintaining test coverage for backward compatibility. </details> <details> <summary>internal/api/v2/common.go (2)</summary> `4-4`: **LGTM! Proper imports added for new functionality.** The `fmt` and `strcase` imports are correctly added to support error formatting and column name case conversion for the new sort parameter functionality. Also applies to: 8-8 --- `94-107`: **Well-implemented sort parameter parsing with proper validation.** The sort parameter parsing logic is correctly implemented: - Uses `SplitN` with limit 2 to handle column names that might contain colons - Converts column names to snake_case for consistency - Validates order values (asc/desc) with case-insensitive comparison - Returns descriptive errors for invalid input The implementation follows good API design practices and handles edge cases properly. </details> <details> <summary>internal/storage/ledger/accounts.go (3)</summary> `8-8`: **LGTM! Proper import added for time functionality.** The time import is correctly added to support the new `at time.Time` parameter in the method signature. --- `25-25`: **Correctly implemented first usage timestamp tracking.** The method signature update and FirstUsage field assignment properly implement the new feature: - The `at time.Time` parameter allows callers to specify the timestamp - Setting `FirstUsage: at` ensures the timestamp is captured for new accounts This change aligns with the broader API enhancement for tracking account first usage. Also applies to: 49-49 --- `61-61`: **Excellent SQL logic for preserving earliest first usage timestamp.** The upsert logic correctly uses a CASE statement to preserve the earliest `first_usage` value: ```sql first_usage = case when excluded.first_usage < accounts.first_usage then excluded.first_usage else accounts.first_usage endThis ensures that the first usage timestamp is never overwritten with a later timestamp, maintaining data integrity for historical tracking.
internal/storage/common/errors.go (1)
24-41: Well-designed error type following Go best practices.The
ErrNotPaginatedFielderror type is properly implemented:
- Contains contextual information (field name) for debugging
- Implements standard
Error()method with clear, descriptive message- Includes
Is()method for proper error comparison and handling- Uses unexported constructor function to maintain API consistency
This error type will effectively support the new pagination validation features.
internal/controller/ledger/store_generated_test.go (1)
314-316: LGTM! Mock implementation correctly updated to match interface changes.The generated mock method signature has been properly updated to include the new
at time.Timeparameter, maintaining consistency with theStoreinterface changes. The mock recorder methods are also correctly updated to handle the additional parameter.Also applies to: 322-324
pkg/client/models/components/v2account.go (4)
5-9: LGTM! Proper imports added for extended functionality.The import updates correctly add the necessary packages:
utilsfor JSON marshaling/unmarshaling consistencytimepackage for the new FirstUsage field
13-13: Correctly implemented optional FirstUsage field.The
FirstUsagefield is properly defined as an optional pointer type (*time.Time) with the correct JSON tag"firstUsage,omitempty". This allows the field to be omitted when null and follows the API specification.
18-27: Proper JSON serialization methods for consistency.The custom
MarshalJSONandUnmarshalJSONmethods correctly delegate to internal utility functions, ensuring consistent JSON handling across the client SDK. This maintains the established patterns in the codebase.
43-48: Well-implemented getter method with nil-safety.The
GetFirstUsage()method follows the established pattern with proper nil-checking, ensuring safe access to the optional FirstUsage field. This maintains consistency with other getter methods in the struct.pkg/client/models/operations/v2listtransactions.go (3)
13-13: LGTM: Proper deprecation handling.The deprecation of the
Ordertype and field is handled correctly with clear messaging directing users to the newsortparameter while maintaining backwards compatibility.Also applies to: 51-54
56-59: LGTM: Well-documented Sort field.The new
Sortfield is properly implemented with clear documentation explaining the expected format and consistent struct tags.
123-128: LGTM: Consistent getter method implementation.The
GetSort()method follows the established pattern used by other getter methods in the struct, with proper nil checking and correct return type.test/e2e/api_accounts_list_test.go (1)
139-141: LGTM: Proper FirstUsage field integration.The
FirstUsagefield has been correctly added to the test assertions and expected account structures, using the appropriate timestamp values.Also applies to: 151-151, 156-156
docs/api/README.md (1)
1962-1976: Logs endpoint: mismatch between example and docsThe text says “sorted by ID in descending order” but the new
sortparam allows overriding this. Explicitly document the current default (id:desc) and note that supplyingsortoverrides it.
| When("paginating on date", func() { | ||
| It("should be ok", func(specContext SpecContext) { | ||
| _, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.ListLogs( | ||
| ctx, | ||
| operations.V2ListLogsRequest{ | ||
| Ledger: "default", | ||
| Sort: pointer.For("date:asc"), | ||
| }, | ||
| ) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing test coverage for sorting functionality.
The current test only verifies that the API accepts the sort parameter without error, but doesn't validate the actual sorting behavior. For comprehensive testing of this new feature, consider adding assertions to verify:
- The response data is actually sorted by date in ascending order
- Different sort parameters work correctly (e.g., "id:desc", "date:desc")
- Error handling for invalid sort parameters
Example enhancement:
When("paginating on date", func() {
It("should be ok", func(specContext SpecContext) {
- _, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.ListLogs(
+ response, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.ListLogs(
ctx,
operations.V2ListLogsRequest{
Ledger: "default",
Sort: pointer.For("date:asc"),
},
)
Expect(err).ToNot(HaveOccurred())
+
+ // Verify logs are sorted by date in ascending order
+ logs := response.V2LogsCursorResponse.Cursor.Data
+ for i := 1; i < len(logs); i++ {
+ Expect(logs[i].Date.After(logs[i-1].Date) || logs[i].Date.Equal(logs[i-1].Date)).To(BeTrue())
+ }
})
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| When("paginating on date", func() { | |
| It("should be ok", func(specContext SpecContext) { | |
| _, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.ListLogs( | |
| ctx, | |
| operations.V2ListLogsRequest{ | |
| Ledger: "default", | |
| Sort: pointer.For("date:asc"), | |
| }, | |
| ) | |
| Expect(err).ToNot(HaveOccurred()) | |
| }) | |
| }) | |
| When("paginating on date", func() { | |
| It("should be ok", func(specContext SpecContext) { | |
| response, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.ListLogs( | |
| ctx, | |
| operations.V2ListLogsRequest{ | |
| Ledger: "default", | |
| Sort: pointer.For("date:asc"), | |
| }, | |
| ) | |
| Expect(err).ToNot(HaveOccurred()) | |
| // Verify logs are sorted by date in ascending order | |
| logs := response.V2LogsCursorResponse.Cursor.Data | |
| for i := 1; i < len(logs); i++ { | |
| Expect(logs[i].Date.After(logs[i-1].Date) || logs[i].Date.Equal(logs[i-1].Date)).To(BeTrue()) | |
| } | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In test/e2e/api_logs_list_test.go around lines 126 to 137, the test only checks
that the API accepts the sort parameter without error but does not verify the
sorting behavior. Enhance the test by adding assertions to confirm the response
data is sorted by date ascending, add additional test cases for other sort
parameters like "id:desc" and "date:desc", and include tests for error handling
when invalid sort parameters are provided.
Fixes LX-62